Conversation
dkhalanskyjb
force-pushed
the
year-month
branch
2 times, most recently
from
a58f920 to
c42a067
Compare
dkhalanskyjb
force-pushed
the
year-month
branch
2 times, most recently
from
112e847 to
23b69c8
Compare
|
About time we get this added. You need to resolve merge conflicts |
|
You are right! Thanks for the advice! |
dkhalanskyjb
force-pushed
the
year-month
branch
from
23b69c8 to
186a5fc
Compare
core/common/src/YearMonth.kt
Outdated
| * | ||
| * @sample kotlinx.datetime.test.samples.YearMonthSamples.days | ||
| */ | ||
| // val days: LocalDateRange get() = firstDay..lastDay // no ranges yet |
There was a problem hiding this comment.
What is the reason for commenting this out? Seems useful to me.
There was a problem hiding this comment.
The reason is that LocalDateRange is not in master. See the description of the pull request.
There was a problem hiding this comment.
While LocalDateRange may not be in master, you can for now implement this as:
val days: ClosedRange<LocalDate> get() = firstDay..lastDay
There was a problem hiding this comment.
Actually, as of 2 days ago it looks like LocalDateRange is in master
core/common/src/YearMonth.kt
Outdated
| * | ||
| * @sample kotlinx.datetime.test.samples.YearMonthSamples.days | ||
| */ | ||
| // val days: LocalDateRange get() = firstDay..lastDay // no ranges yet |
There was a problem hiding this comment.
Another useful range would be an open end range of LocalDateTime that is midnight on the first day of the given month until midnight on the first day of the next month
There was a problem hiding this comment.
There are no ranges of LocalDateTime semantically, because due to DST transitions, sometimes, "earlier" local date-times follow the "later" ones, and some local date-times don't exist at all.
There was a problem hiding this comment.
LocalDateTime implements Comparable so technically anything Comparable can be a range. Your argument is really that LocalDateTime should not be Comparable
| * @throws DateTimeArithmeticException if the result exceeds the boundaries of [YearMonth]. | ||
| * @sample kotlinx.datetime.test.samples.YearMonthSamples.minusMonth | ||
| */ | ||
| public fun YearMonth.minusMonth(): YearMonth = minus(1, DateTimeUnit.MONTH) |
There was a problem hiding this comment.
These 4 functions should be plural and accept an int parameter that defaults to 1 as in:
public fun YearMonth.minusMonths(months: Int = 1): YearMonth = minus(months, DateTimeUnit.MONTH)
There was a problem hiding this comment.
In our research, we haven't found cases where the value was anything other than 1, so the extra flexibility is pointless. You have a more general plus for this case if 1 doesn't suit your needs.
core/common/src/YearMonth.kt
Outdated
| * | ||
| * @sample kotlinx.datetime.test.samples.YearMonthSamples.firstAndLastDay | ||
| */ | ||
| public val lastDay: LocalDate get() = onDay(numberOfDays) |
There was a problem hiding this comment.
You should add contains methods for LocalDate and LocalDateTime as in:
public fun contains(date: LocalDate): Boolean = date in firstDay..lastDay
Also helpful would be intersects functions for ranges to determine if this year month has any overlap with another range. This should include the cases of ClosedRange<LocalDate> and OpenEndRange<LocalDateTime>.
public fun intersects(dateRange: ClosedRange<LocalDate>): Boolean = ...
public fun intersects(dateTimeRange: OpenEndRange<LocalDateTime>): Boolean = ...
There was a problem hiding this comment.
Nope, we've decided that to interpret the YearMonth as a range, you'd use the days function (currently commented out).
dkhalanskyjb
force-pushed
the
year-month
branch
4 times, most recently
from
8929852 to
77c4d23
Compare
dkhalanskyjb
force-pushed
the
year-month
branch
2 times, most recently
from
4675ea7 to
d2e5542
Compare
|
|
||
| /** | ||
| * Combines this year-month with the specified day-of-month into a [LocalDate] value. | ||
| * |
There was a problem hiding this comment.
Makes sense to specify an exception if the day value is outside of the range 1..numberOfDays
core/common/src/YearMonth.kt
Outdated
| * | ||
| * ### Platform specifics | ||
| * | ||
| * The range of supported years is platform-dependent, but at least is enough to represent year-months of all instants |
There was a problem hiding this comment.
Is it still true after we unified ranges across platforms?
3 participants
Fixes #168
Fixes #184